Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize HTML before comparison #26

Closed
wants to merge 2 commits into from
Closed

Normalize HTML before comparison #26

wants to merge 2 commits into from

Conversation

geoffliu
Copy link

@geoffliu geoffliu commented Jul 27, 2016

An alternative fix to @Treora's PR.

@slorber
Copy link
Collaborator

slorber commented Jul 27, 2016

that seems nice (solving #18)

not sure but could we avoid normalizing if possible?
ie if there's already a match with unnormalized maybe we can avoid normalizing.

I'm thinking of something better: maybe when render() is called we should store some this.renderedHtml = html property, and then use it in shouldComponentUpdate instead of node.innerHtml? this would probably fix the problem

@slorber
Copy link
Collaborator

slorber commented Jul 27, 2016

Not sure but I also think calling innerHtml has a cost

@geoffliu
Copy link
Author

@slorber The whole idea of this component is that shouldComponentUpdate will return false, and we're skipping render calls. Hence we must compare the input prop to the inner HTML in the live DOM. I pushed another change to bypass the normalization though, in cases where the browser does not reformat.

@Treora
Copy link
Collaborator

Treora commented Jul 27, 2016

Looks like an equally valid workaround to me. I however do not see anymore why we need such a workaround at all. Maybe I am too sleepy, but the 'something better' solution slorber suggested sounds sound to me now.

The reason is that browser reformatting should not be a problem at all. Assume we render some html, and the browser reformats it. There are then two ways the html prop may change:

  1. By the user's edit, in which case nextProps.html already incorporates the browser's reformatting and the nextProps.html !== this.htmlEl.innerHTML test adequately cancels the rerender.
  2. Programatically, in which case we have to rerender anyway, if it is different from what we rendered last time.

So just adding a check whether the html is any different from last time should be enough. We do not even need to store it in this.renderedHtml or anything, since the previous this.props is still available in shouldComponentUpdate. I will put it in a PR. :)

@lovasoa
Copy link
Owner

lovasoa commented Jul 27, 2016

Hi all! I don't use this project myself anymore, and don't really have time for it at the moment. You may all be more knowledgeable about this module than I am.

Do some of you have the time and goodwill to help me maintain this repo on github?

@slorber
Copy link
Collaborator

slorber commented Aug 1, 2016

same for me unfortunatly I can only provide feedback from my own experience

@geoffliu
Copy link
Author

geoffliu commented Aug 1, 2016

Heh, our project ended up taking pieces of this code and adapting it. We had some unfortunate interactions that didn't fit this model.

@slorber
Copy link
Collaborator

slorber commented Aug 1, 2016

any example? maybe you are not alone (I don't use react-contenteditable either)

@lovasoa
Copy link
Owner

lovasoa commented Aug 2, 2016

So, is this PR outdated by #27 ? Can I close it ?

@Treora
Copy link
Collaborator

Treora commented Aug 3, 2016

If I am not mistaken this should indeed be outdated by #27.

Regarding maintainance, I could try help out at least while using the module in my own project.

@Treora Treora closed this Sep 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants